cygthread: suspend thread before terminating. #234
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This addresses an extremely difficult to debug deadlock when running under emulation on ARM64. A relatively easy way to trigger this bug is to call `fork()`, then within the child process immediately call another `fork()` and then `exit()` the intermediate process. It would seem that there is a "code emulation" lock on the wait thread at this stage, and if the thread is terminated too early, that lock still exists albeit without a thread, and nothing moves forward. It seems that a `SuspendThread()` combined with a `GetThreadContext()` (to force the thread to _actually_ be suspended, for more details see https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743) makes sure the thread is "booted" from emulation before it is suspended. Hopefully this means it won't be holding any locks or otherwise leave emulation in a bad state when the thread is terminated. Also, attempt to use `CancelSynchonousIo()` (as seen in `flock.cc`) to avoid the need for `TerminateThread()` altogether. This doesn't always work, however, so was not a complete fix for the deadlock issue. Addresses: https://cygwin.com/pipermail/cygwin-developers/2024-May/012694.html Signed-off-by: Jeremy Drake <[email protected]>
This was referenced Nov 13, 2024
lazka
added a commit
to lazka/MSYS2-packages
that referenced
this pull request
Nov 13, 2024
lazka
added a commit
to msys2/MSYS2-packages
that referenced
this pull request
Nov 13, 2024
|
This sadly results in lots of errors being printed (w11 26100): |
|
D'oh. That error is expected due to C:\>net helpmsg 995
The I/O operation has been aborted because of either a thread exit or an application request.I wonder why I didn't see that when I tested this on ARM64 |
This comment was marked as outdated.
This comment was marked as outdated.
jeremyd2019
added a commit
to jeremyd2019/winautoconfig
that referenced
this pull request
Nov 14, 2024
After msys2/msys2-runtime#234, we shouldn't hit msys2/msys2-autobuild#62 anymore, so remove the workarounds for it. Also remove the sed for enabling clangarm64 in pacman.conf, since it has been enabled by default for a couple of years now.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses an extremely difficult to debug deadlock when running under emulation on ARM64.
A relatively easy way to trigger this bug is to call
fork(), then within the child process immediately call anotherfork()and thenexit()the intermediate process.It would seem that there is a "code emulation" lock on the wait thread at this stage, and if the thread is terminated too early, that lock still exists albeit without a thread, and nothing moves forward.
It seems that a
SuspendThread()combined with aGetThreadContext()(to force the thread to actually be suspended, for more details see https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743) makes sure the thread is "booted" from emulation before it is suspended.Hopefully this means it won't be holding any locks or otherwise leave emulation in a bad state when the thread is terminated.
Also, attempt to use
CancelSynchonousIo()(as seen inflock.cc) to avoid the need forTerminateThread()altogether. This doesn't always work, however, so was not a complete fix for the deadlock issue.Addresses: https://cygwin.com/pipermail/cygwin-developers/2024-May/012694.html
Fixes msys2/msys2-autobuild#62, fixes #228 (and some other issues scattered about I don't remember off-hand)
Awaiting review on cygwin-patches mailing list: https://inbox.sourceware.org/cygwin-patches/[email protected]/T/#u